PROTON-2307 Allow access to connection properties in cpp binding#278
PROTON-2307 Allow access to connection properties in cpp binding#278pjfawcett wants to merge 1 commit intoapache:masterfrom
Conversation
|
The AppVeyor "Visual Studio 2015" build failed due to a problem with Microsoft vcpkg. It looks like this has now been fixed |
|
I don't see option to rerun Appveyor, so the empty commit or empty --amend is probably the only way. |
pjfawcett
force-pushed
the
PROTON-2307-allow-access-to-connection-properties
branch
from
13caa7b to
1e7d52f
Compare
|
This is my first Qpid Pull Request hence I am unfamiliar with the process and would welcome some advice. Firstly, what should I do about the failed builds? They seem to be related to errors/problems in the build system(s) rather than to changes I have made, but I might be missing something. Secondly, and connected, do I need to get this PR to pass all the build checks before it is eligible for review, or is there something else I have to do (apart from being more patient) ? Thanks |
|
I believe some of the builds are a little flakey in CI (and there are also some tests enabled in CI that aren't in the default build because of that). If you are confident the failures aren't from your changes then feel free to just say so. Another approach is to give the build a prod and see if anything changes (e.g git commit --amend --reset-author to redo the commit, followed by a force push). You don't need to get the tests passing to elicit a review, you just need someone with the requisite knowledge to actually come along (which is not me I'm afraid). |
|
Thanks Robbie. I'll force another push. I was wary of doing it too repeatedly having seen some recent messages about Travis. CI limits (though later messages suggest that isn't relevant) |
pjfawcett
force-pushed
the
PROTON-2307-allow-access-to-connection-properties
branch
from
1e7d52f to
d58c0a9
Compare
astitcher
left a comment
astitcher
left a comment
There was a problem hiding this comment.
@pjfawcett Firstly many apologies it has taken me so long to review this PR.
I really think this is a useful addition to the API.
I spent a while thinking about whether using std::map<> was a good idea in the context of this API, and I concluded that it makes sense even if it is a rather heavyweight object to be returning. Another alternative might be to just return a proton::map<> which is comparatively lightweight, but not all that convenient without first converting it to a std::map just as you did internally.
I've a few necessary changes below , but otherwise I think this is good essentially as-is.
I also note that session and link objects also have properties (but these maybe not as useful as the connection properties).
If you rebase this change on master I think you can drop the Visual Studio hacks and the tests should pass as appveyor have now fixed their VS2015 image.
pjfawcett
force-pushed
the
PROTON-2307-allow-access-to-connection-properties
branch
from
19d3080 to
f5a911d
Compare
|
@astitcher regarding properties on session and link: I'd be happy to implement similar access to those if it was thought to be useful - probably under a follow-on ticket? |
That would be great! I'll try to get to it much sooner next time! |
|
@pjfawcett If you squash rebase these 2 commits then I'll be happy to merge. (I can do it too, but it'll be a little easier for you) |
Allow access to properties on incoming connections
pjfawcett
force-pushed
the
PROTON-2307-allow-access-to-connection-properties
branch
from
f5a911d to
55ad0e8
Compare
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
==========================================
+ Coverage 79.56% 84.23% +4.66%
==========================================
Files 343 46 -297
Lines 43066 2341 -40725
==========================================
- Hits 34267 1972 -32295
+ Misses 8799 369 -8430
Continue to review full report at Codecov.
|
|
Squashed into single commit. |
pjfawcett
deleted the
PROTON-2307-allow-access-to-connection-properties
branch
pjfawcett
restored the
PROTON-2307-allow-access-to-connection-properties
branch
…tions Allow access to properties on incoming connections closes apache#278
5 participants
Allow setting of connection properties via connection_options
Allow reading of properties on incoming connections
Add unit test to container_test.cpp